Improve performance by caching src/dest when applying resource manifests#1235
Conversation
jerpeter1
force-pushed
the
avoid-deep-copy
branch
from
7648d23 to
09cd7b0
Compare
|
/hold |
openshift-ci
Bot
added
do-not-merge/hold
|
/unhold |
|
@deads2k - can you please take a look at this PR? |
|
/assign deads2k |
| } | ||
|
|
||
| // map of name,kind to resourceHash (of last applied resource) : resourceVersion(when resource was applied) | ||
| var cachedResourceVersions map[CachedVersionKey]CachedResource = make(map[CachedVersionKey]CachedResource) |
There was a problem hiding this comment.
no global state please. I'd rather either
- break everyone and change the methods
- create new methods that accept a cache or use a method receiver struct to track it and have the staticresource controller maintain them.
There was a problem hiding this comment.
I chose option 2. Since there are so many users of resourceapply.ApplyFoo functions, I decided to create new functions that accept a cache and use those new functions from static resource controller, buta t some point I'd like to go back and add something that lets everyone take advantage of the enhanced performance.
| } | ||
|
|
||
| type CachedResource struct { | ||
| resourceHash, resourceVersion string |
There was a problem hiding this comment.
for future readers can you godoc these. I think that resourcehash is the hash of the required in an ApplyFoo that is computed in case the input changes and resourceVersion is the received resourceVersion from the apiserver in response to an update that is comparable to the GET.
| // map of name,kind to resourceHash (of last applied resource) : resourceVersion(when resource was applied) | ||
| var cachedResourceVersions map[CachedVersionKey]CachedResource = make(map[CachedVersionKey]CachedResource) | ||
|
|
||
| func updateCachedResourceVersion(name string, kind string, namespace string, resourceHash string, resourceVersion string) { |
There was a problem hiding this comment.
if this took a runtime.Object instead of a resourceVersion, you'd be able to simply call updateCachedResourceVersion and have it clear on nil (or do nothing) and avoid all the nil checks in callers.
| // if the manifest applied is the same as the last time this resource was | ||
| // updated, and the resourceVersion also hasn't changed, we don't need to | ||
| // actually do anything | ||
| func checkCachedResourceVersion(name string, kind string, namespace string, resourceHash string, resourceVersion string) bool { |
There was a problem hiding this comment.
hasResourceMutated or resourceChanged or mayNeedUpdate?
There was a problem hiding this comment.
I went with safeToSkipApply
| // ApplyNamespace merges objectmeta, does not worry about anything else | ||
| func ApplyNamespace(ctx context.Context, client coreclientv1.NamespacesGetter, recorder events.Recorder, required *corev1.Namespace) (*corev1.Namespace, bool, error) { | ||
| existing, err := client.Namespaces().Get(ctx, required.Name, metav1.GetOptions{}) | ||
| resourceHash := hashOfResourceStruct(required) |
There was a problem hiding this comment.
While technically more efficient, this requires callers to compute instead of simply passing their required to checkCachedResourceVersion and updateCachedResourceVersion. My preference is to pass the runtime.Objects around and know that if we actually issued an update, it was significantly more expensive than re-computing the hash.
There was a problem hiding this comment.
Done. It is less performant than the previous implementation (since we're calculating the hash of required several times from the same ApplyFoo, but it's still an improvement from the prior implementation.
jerpeter1
force-pushed
the
avoid-deep-copy
branch
from
08fdd66 to
af6236b
Compare
openshift-ci
Bot
added
the
needs-rebase
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: jerpeter1 The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
jerpeter1
force-pushed
the
avoid-deep-copy
branch
from
af6236b to
0bb9138
Compare
openshift-ci
Bot
removed
the
needs-rebase
|
/retest-required |
…when applying resource manifests, and skipping updating the apiserver when it won't do anything
jerpeter1
force-pushed
the
avoid-deep-copy
branch
from
0bb9138 to
7bcd828
Compare
|
@jerpeter1: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
3 participants
If we see that neither the manifest being applied (by caching a hash of the manifest),
or the existing resource in the cluster (by caching the resourceVersion) has
changed since the manifest was last updated, applying the resource can be
skipped, since it won't actually do anything.